-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cherry pick omas viewer dev #256
Conversation
…y_pick_omas_viewer_dev
The regression test fails because of errors that are likely in OMFIT:
What OMFIT version does the test environment use? |
This has been tested as part of #257 and seems to work for that case. |
Stale pull request message |
I tried following a series of PRs here on omas that all depend on each other. Can @AreWeDreaming open an issue here on omas with the correct order of operations for merging? Then we can tackle each one sequentially. |
This is the PR that needs to be merged next. |
- Plus 2 missed density_thermal
I don't see any reviewers assigned. Is this still waiting on someone before the merge? |
Congratulations @bechtt you have fallen to one of my classic blunders. |
Need to create OMFIT PR with |
It sounds like the plan is to hold off on the electron density vs density_thermal change until we hear some clarification from IMAS devs about the possible use cases for this (through JIRA) |
@AreWeDreaming I would think that |
@jmcclena do you understand why density_thermal would be useful for electrons? It seems like it only makes sense for ions |
While not as well understood as fast ions, I believe suprathermal instabilities like fishbones have been observed with ECH heating (https://iopscience.iop.org/article/10.1088/0029-5515/47/11/022/pdf). You could also have a fast runaway electron population near a disruption. Additionally, while there is no fast electron temperature, there is a fast parallel and perp pressure (just like how the the ions are set up) |
@jmcclena I did my thesis on non-thermal electron distributions functions generated by ECH. You have to try very hard (core density 1.e19 + 0.8 MW ECH) to generate non-thermal electron distributions, and even then they make up less than 1 % of the electron density. |
Shouldn't fast (and total) electron density also be set according to ITER's recommendation? https://jira.iter.org/browse/IMAS-5050 |
Probably but that is out of scope for this PR |
How so? It looks like you are adding the mappings to omas from MDS+ for electron density. What would be a more appropriate time to set up the proper mapping? |
Well to do it properly every time electron density_thermal is set we would need to set density_fast as well. I could do that but we have 4 more PRs to work through. |
ods['core_profiles.profiles_1d.0.electrons.density_thermal'].xarray() | ||
ods['core_profiles.profiles_1d.0.electrons.densdensity_thermality'].xarray() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a typo
I tried my best to test this (and the original branch) with omas viewer, but couldn't get any plots to work. That probably isn't an ideal test here (since the development has stalled), but I don't have anything better. It could be worth running the OMFIT regression tests with this omas branch to ensure nothing breaks there. If that works I'd be comfortable with merging this |
Stale pull request message |
This passed 73 OMFIT regression tests here https://github.com/gafusion/OMFIT-source/pull/6967 and all omas tests so I don't see any reason not to merge |
This cherry picked the changes from
omas_viewer_dev
.omas_machine
(only tested for d3d)core_profile
data fromOMFITprofiles
treeomas_plot
to accommodate requirements of OMAS viewer.electron.density_thermal
toelectron.density
throughout OMAS. This needs to be propagated to OMFIT.omas_physics
andomas_plot
that were stopping regression from working.Ready for review